fix(rendering): area, line and point rendered on the same layer for each series #290
Conversation
This commit change the rendering order of the line series. Now we render: for each series the line and each data point, instead of rendering first all the lines and than all the points fix elastic#287
Codecov Report
@@ Coverage Diff @@
## master #290 +/- ##
==========================================
- Coverage 98.17% 98.14% -0.03%
==========================================
Files 37 37
Lines 2682 3019 +337
Branches 617 759 +142
==========================================
+ Hits 2633 2963 +330
- Misses 44 49 +5
- Partials 5 7 +2
Continue to review full report at Codecov.
|
|
|
||
| getLineToRender(glyph: LineGeometry, sharedStyle: SharedGeometryStyle, index: number) { | ||
| const { line, color, transform, geometryId, seriesLineStyle } = glyph; | ||
| const key = `line-${index}`; |
There was a problem hiding this comment.
Can we use something other than the index for the key?
|
|
||
| if (this.props.animated) { | ||
| return ( | ||
| <Group key={index} x={transform.x}> |
There was a problem hiding this comment.
Also here? re: index
| return ( | ||
| <Group key={index} x={transform.x}> | ||
| <Spring native reset from={{ opacity: 0 }} to={{ opacity: 1 }}> | ||
| {() => { |
There was a problem hiding this comment.
Can we extract this function so it doesn't create a new function on each rerender? I don't think it will be that different on performance but best to keep up with best practices.
There was a problem hiding this comment.
removed the animation in 5d712a5
Depending if stacked or not, we draw first all the stacked areas (fist all the areas, then lines then points). For non stacked elements, we draw groups of area, line, points.
markov00
changed the title
There was a problem hiding this comment.
This looks a lot better. I think removing the animations is a good idea for now.
Just have a few comments about the index in the string. I think including that in the key string will cause the component to rerender if the index value changes in the future, at least that's my understanding of react. If the key would be unique without the index I think it should be removed.
|
🎉 This PR is included in version 9.0.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [9.0.3](elastic/elastic-charts@v9.0.2...v9.0.3) (2019-08-13) ### Bug Fixes * zIndex order for areas, lines and points ([opensearch-project#290](elastic/elastic-charts#290)) ([4552c45](elastic/elastic-charts@4552c45)), closes [opensearch-project#287](elastic/elastic-charts#287)
Summary
This commit change the rendering order of the line series and area series.
For lines we render: for each series the line and each data point, instead of rendering first all the lines and than all the points.
For areas, depending if the area is stacked or not:
This PR also remove completely the animation from lines and areas as its currently disabled in the library. We will reintegrate the animation in a future release.
fix #287
before
after
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] Any consumer-facing exports were added tosrc/index.ts(and stories only import from../srcexcept for test data & storybook)[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Proper documentation or storybook story was added for features that require explanation or tutorials[ ] Unit tests were updated or added to match the most common scenarios